-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add differentiation between LTS and non-LTS builds #1737
Conversation
Download the artifacts for this pull request here: GUI:
CLI: |
Great, it works |
THIS GIT WORKS WITH THE LATEST ANTON BLAST DEMO!!! |
W pull request |
UndertaleModLib/UndertaleData.cs
Outdated
/// <param name="isLTS">true for LTS, false for non-LTS.</param> | ||
public void SetLTS(bool isLTS) | ||
{ | ||
if (isLTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this gonna handle stuff when we have different LTS versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll definitely still require adjustment. IsVersionAtLeast might work, or otherwise I could integrate it into an override for SetGMS2Version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latter is probably nicer yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me with this structure. Before we merge/approve this, I just have a few tiny nitpicks I left comments about, and we should test this on as many games as possible. I'll get to doing so with games I have handy myself momentarily.
UndertaleModLib/UndertaleData.cs
Outdated
{ | ||
if (major != 2 && major != 2022 && major != 2023 && major != 2024) | ||
throw new NotSupportedException("Attempted to set a version of GameMaker " + major + " using SetGMS2Version"); | ||
|
||
GeneralInfo.Major = major; | ||
GeneralInfo.Major = (uint)major; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this cast can be removed now
/// </summary> | ||
public enum BranchType | ||
{ | ||
Unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looking at the other fields, we could just name this to something like PreLTS2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can technically remain as this even through the end of deserialization on 2022.0.1... but the same is going to apply for "PostLTS2022" with 2025.0, so fair enough.
} | ||
|
||
/// <summary> | ||
/// The GameMaker release branch of the data file. May be set to "NonLTS" when features exempted from LTS are detected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"NonLTS" doesn'T exist. please adjust documentation (and use cref tags if it makes sense=
sb.Append(Major); | ||
sb.Append('.'); | ||
sb.Append(Minor); | ||
if (Branch == BranchType.LTS2022_0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: have some way to get these dynamically from the enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some testing, and it seems to work! I'm also satisfied with the current iteration.
Description
After the end of October 2022, the GameMaker versions were split into two: LTS and non-LTS. This marked the beginning of the era called "UndertaleModTool breaking for the 400th time".
This PR adds a Branch enum value to GeneralInfo which can be used to differentiate between these versions without causing confusion with
UndertaleData.IsVersionAtLeast()
(as actually setting the version to 2022.0 would indicate being much older than many of the features therein). It also adds some methods to interact with this, one beingUndertaleData.IsNonLTSVersionAtLeast()
, and implements its usage in regards to Particle Systems and Spine Sprites, both of which are not present on the current LTS despite other fixes. Closes #1703, closes #1729.Caveats
Using an enum is theoretically extensible for other branches, but that seems improbable. More likely we'll just get 2024.0 and have to adjust around that; mostly I just don't want to go back to version booleans. The
UndertaleData.SetLTS()
method is also weird (boolean input), and this all renders the estimated version in the window title completely off. Oh, and it can't be set through the GUI.Notes
Due to all the caveats (and wanting further input on how to resolve them), I'm marking this as draft.